-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/TS expiration handling without network #2937
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2937 +/- ##
==========================================
+ Coverage 23.90% 24.01% +0.10%
==========================================
Files 776 773 -3
Lines 45708 45597 -111
==========================================
+ Hits 10928 10949 +21
+ Misses 33920 33787 -133
- Partials 860 861 +1 ☔ View full report in Codecov by Sentry. |
8831c47
to
8314653
Compare
It takes too much of network/disk to handle tombstones expiration and removal, so it is better to store it when tombstone is being indexed in metabase. Additional little-endian expiration suffix was added to graveyard values. Metabase version was increased as it is a non-compatible change. Relates #2929. Signed-off-by: Pavel Karpy <[email protected]>
They do not differ from the other objects (e.g. locks do). Initial logic has changed much, graveyard now allows to handle expired tombstones marks (do not confuse it with the lists of regular indexes) independently, while disk can be cleared with the other types of object. Also, add tests for it. Signed-off-by: Pavel Karpy <[email protected]>
Graveyard now has tombstone expiration marks in epochs, there is no need to use any network requests, just drop records if an epoch is big enough. Closes #2929. Signed-off-by: Pavel Karpy <[email protected]>
Signed-off-by: Pavel Karpy <[email protected]>
It may or may not index tombstone's expiration in graveyard. Signed-off-by: Pavel Karpy <[email protected]>
Signed-off-by: Pavel Karpy <[email protected]>
8314653
to
152150e
Compare
Migration can be done automatically, let admin's life be a better thing. TS expiration is taken as the current epoch + 5. It is not critical if TS will live more. In practice, side effects _may_ be seen only the first 5 hours after an update, and only returned status code _may_ differ. Signed-off-by: Pavel Karpy <[email protected]>
@@ -49,7 +58,7 @@ func checkVersion(tx *bbolt.Tx, initialized bool) error { | |||
return nil | |||
} | |||
|
|||
func updateVersion(tx *bbolt.Tx, version uint64) error { | |||
func (db *DB) updateVersion(tx *bbolt.Tx, version uint64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why making it a method if db
is never used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think it's good to separate commit dropping old implementation from the one changing the approach. Although red diff is pretty, transient state with dead code makes no sense
defer db.modeMtx.RUnlock() | ||
|
||
if db.mode.NoMetabase() { | ||
return -1, ErrDegradedMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont see any sense in negative return, can be zero?
if binary.LittleEndian.Uint64(v[addressKeySize:]) < epoch { | ||
err := c.Delete() | ||
if err != nil { | ||
return fmt.Errorf("cleared %d TS marks in %d epoch and got error: %w", counter, epoch, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id add context to the error outside db handler
k, v := c.First() | ||
|
||
for k != nil { | ||
if binary.LittleEndian.Uint64(v[addressKeySize:]) < epoch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd still check v
length explicitly to avoid unclear panic, especially since the suffix was not always there
var counter int | ||
|
||
err := db.boltDB.Update(func(tx *bbolt.Tx) error { | ||
bkt := tx.Bucket(graveyardBucketName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if bkt == nil
?
return res, fmt.Errorf("decode tombstone address from value: %w", err) | ||
} | ||
|
||
if len(v) == addressKeySize+8 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>
case should be either erroneous or no-problem, silence is bad
|
||
func migrateFrom2Version(db *DB, tx *bbolt.Tx) error { | ||
tsExpiration := db.epochState.CurrentEpoch() + objectconfig.DefaultTombstoneLifetime | ||
bkt := tx.Bucket(graveyardBucketName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again can be nil
|
||
for k, v := c.First(); k != nil; k, v = c.Next() { | ||
newVal := make([]byte, addressKeySize, addressKeySize+8) | ||
copy(newVal, v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo we should assert len(v) == addressKeySize
No description provided.